feat: [v0.8-develop] Add allowlist sample hook, refactor test base #70

adamegyed · 2024-06-11T18:42:05Z

Motivation

To provide better context for how pre-validation hooks should be used, it would help to provide some real use cases. A simple case is an allowlist plugin - limiting the addresses, and possibly the selectors, of the targets of execute and executeBatch when using a specific validation function.

Solution

Implement a simple allowlist hook plugin.
Update the (temporary) custom validation initialization workflow for more flexibility.
Provide a fuzz test suite to ensure it works correctly.
To simplify existing and new tests:
- Pull some common workflows for constructing user ops and runtime calls out to AccountTestBase.
- Add a new extension to AccountTestBase called CustomValidationTestBase, and update the Per-hook data test (and the allowlist test) to use this.

Zer0dot · 2024-06-12T13:03:18Z

src/samples/permissionhooks/AllowlistPlugin.sol

+        override
+        returns (uint256)
+    {
+        if (functionId == uint8(FunctionId.PRE_VALIDATION_HOOK)) {


What's the point of this check, and the similar one in the runtime validation hook?

In other plugin examples we've been using an if-else chain for the internal function dispatcher, so I was following that pattern here too. I suppose we could cut it and just treat every function id the same here, because this one doesn't have multiple implementations.

~~Like the idea of not having the functionId, especially the plugins are global singleton.~~

Actually, we should be explicit since we do ask the functionId var to be passed.

Zer0dot · 2024-06-12T13:15:36Z

src/samples/permissionhooks/AllowlistPlugin.sol

+    }
+
+    // solhint-disable-next-line no-empty-blocks
+    function pluginManifest() external pure override returns (PluginManifest memory) {}


Is this blank because it's supposed to only be installed with a user-provided config?

Yep, basically. The manifest is empty because this plugin doesn't define anything that the manifest can install:

execution functions

execution hooks

validation functions

signature validators

call permissions

fangting-alchemy · 2024-06-18T23:49:30Z

test/account/PerHookData.t.sol

-        );
-
-        vm.deal(address(account1), 100 ether);
+        _customValidationSetup();


Do the old tests here test allowList fails?
Are the error thrown from AllowlistPlugin captured as AA errors?

Yes, depending on the call path, they throw either user op or runtime errors, and the fuzzer expects them to be called.

The error to look for is loaded from here:

reference-implementation/test/samples/AllowlistPlugin.t.sol

Lines 142 to 199 in 8c7133c

function _getExpectedUserOpError(Call[] memory calls) internal view returns (bytes memory) {

for (uint256 i = 0; i < calls.length; i++) {

Call memory call = calls[i];

(bool allowed, bool hasSelectorAllowlist) =

allowlistPlugin.targetAllowlist(call.target, address(account1));

if (allowed) {

if (

hasSelectorAllowlist

&& !allowlistPlugin.selectorAllowlist(call.target, bytes4(call.data), address(account1))

) {

return abi.encodeWithSelector(

IEntryPoint.FailedOpWithRevert.selector,

0,

"AA23 reverted",

abi.encodeWithSelector(AllowlistPlugin.SelectorNotAllowed.selector)

);

}

} else {

return abi.encodeWithSelector(

IEntryPoint.FailedOpWithRevert.selector,

0,

"AA23 reverted",

abi.encodeWithSelector(AllowlistPlugin.TargetNotAllowed.selector)

);

}

}

return "";

}

function _getExpectedRuntimeError(Call[] memory calls) internal view returns (bytes memory) {

for (uint256 i = 0; i < calls.length; i++) {

Call memory call = calls[i];

(bool allowed, bool hasSelectorAllowlist) =

allowlistPlugin.targetAllowlist(call.target, address(account1));

if (allowed) {

if (

hasSelectorAllowlist

&& !allowlistPlugin.selectorAllowlist(call.target, bytes4(call.data), address(account1))

) {

return abi.encodeWithSelector(

UpgradeableModularAccount.PreRuntimeValidationHookFailed.selector,

address(allowlistPlugin),

uint8(AllowlistPlugin.FunctionId.PRE_VALIDATION_HOOK),

abi.encodeWithSelector(AllowlistPlugin.SelectorNotAllowed.selector)

);

}

} else {

return abi.encodeWithSelector(

UpgradeableModularAccount.PreRuntimeValidationHookFailed.selector,

address(allowlistPlugin),

uint8(AllowlistPlugin.FunctionId.PRE_VALIDATION_HOOK),

abi.encodeWithSelector(AllowlistPlugin.TargetNotAllowed.selector)

);

}

}

And the test expects them here:

reference-implementation/test/utils/AccountTestBase.sol

Line 168 in 8c7133c

vm.expectRevert(expectedRevertData);

reference-implementation/test/utils/AccountTestBase.sol

Line 113 in 8c7133c

vm.expectRevert(expectedRevertData);

fangting-alchemy

LGTM

howydev · 2024-06-21T21:50:23Z

src/samples/permissionhooks/AllowlistPlugin.sol

+    function pluginManifest() external pure override returns (PluginManifest memory) {}
+
+    function _checkAllowlistCalldata(bytes calldata callData) internal view {
+        if (bytes4(callData[:4]) == IStandardExecutor.execute.selector) {


Callout: when executeUserOp is used, we need to check calldata[4:8] instead

If this hook doesn't depend on the contents of executeUserOp, then the account shouldn't send the user op, right? I guess this depends on how we do it, but it seems like the account should know whether to use msg.data or userOp.callData depending on the hook config.

yeah agree that it should be abstracted from plugins. in the current permissions stack, we directly send over userOp.callData which could be abi.encodeCall(...) or abi.encodePacked(executeUserOp.selector, abi.encodeCall(...)) for hooks that don't require UO context, and for hooks that require UO context, they would always receive abi.encodePacked(executeUserOp.selector, abi.encodeCall(...)). Dislike that part and would likely change that though

huaweigu

LGTM. Additionally, we developed a 6900 address book plugin that utilized the pre-validation hook.

adamegyed requested a review from a team June 11, 2024 18:42

Zer0dot reviewed Jun 12, 2024

View reviewed changes

adamegyed force-pushed the adam/per-hook-data branch from 62344f2 to 0e67b55 Compare June 12, 2024 18:48

adamegyed force-pushed the adam/sample-allowlist-hook branch from b8637b4 to 4c147bf Compare June 12, 2024 18:51

adamegyed force-pushed the adam/per-hook-data branch from 0e67b55 to c2da4d0 Compare June 14, 2024 15:57

adamegyed force-pushed the adam/sample-allowlist-hook branch 2 times, most recently from cd1637f to 8c7133c Compare June 14, 2024 16:06

fangting-alchemy reviewed Jun 18, 2024

View reviewed changes

adamegyed force-pushed the adam/per-hook-data branch from 64a9917 to 23cdc39 Compare June 19, 2024 15:06

adamegyed force-pushed the adam/sample-allowlist-hook branch from 8c7133c to d48a9a4 Compare June 19, 2024 15:07

fangting-alchemy approved these changes Jun 20, 2024

View reviewed changes

adamegyed force-pushed the adam/per-hook-data branch from 23cdc39 to 4599a2d Compare June 20, 2024 17:53

adamegyed force-pushed the adam/sample-allowlist-hook branch from d48a9a4 to 9bc6789 Compare June 20, 2024 17:53

adamegyed force-pushed the adam/per-hook-data branch from 4599a2d to 8a09d31 Compare June 20, 2024 20:37

adamegyed force-pushed the adam/sample-allowlist-hook branch from 9bc6789 to 17906b1 Compare June 20, 2024 20:37

adamegyed force-pushed the adam/per-hook-data branch from 8a09d31 to d402361 Compare June 20, 2024 20:53

adamegyed force-pushed the adam/sample-allowlist-hook branch from 17906b1 to e3ab9a2 Compare June 20, 2024 20:53

adamegyed force-pushed the adam/per-hook-data branch from d402361 to 60916fa Compare June 20, 2024 21:01

adamegyed force-pushed the adam/sample-allowlist-hook branch from e3ab9a2 to ae5c79a Compare June 20, 2024 21:05

howydev reviewed Jun 21, 2024

View reviewed changes

huaweigu approved these changes Jun 25, 2024

View reviewed changes

adamegyed force-pushed the adam/sample-allowlist-hook branch from ae5c79a to 5785dba Compare June 26, 2024 16:20

adamegyed force-pushed the adam/per-hook-data branch from 2afe76b to 94b7592 Compare June 26, 2024 16:22

adamegyed force-pushed the adam/sample-allowlist-hook branch from 5785dba to 2a965ac Compare June 26, 2024 16:25

adamegyed force-pushed the adam/per-hook-data branch from 94b7592 to e118421 Compare June 27, 2024 19:27

adamegyed force-pushed the adam/sample-allowlist-hook branch from 2a965ac to 4e0fdc3 Compare June 27, 2024 19:29

adamegyed force-pushed the adam/per-hook-data branch from e118421 to 94a3764 Compare June 27, 2024 20:07

adamegyed force-pushed the adam/sample-allowlist-hook branch from 4e0fdc3 to 18abe0f Compare June 27, 2024 20:10

adamegyed force-pushed the adam/per-hook-data branch from 94a3764 to e4526cc Compare June 28, 2024 15:29

adamegyed force-pushed the adam/sample-allowlist-hook branch from 18abe0f to 17ed4e0 Compare June 28, 2024 15:30

adamegyed force-pushed the adam/per-hook-data branch from e4526cc to 68a5847 Compare June 28, 2024 18:38

adamegyed force-pushed the adam/sample-allowlist-hook branch from 17ed4e0 to 78ae1eb Compare June 28, 2024 18:38

adamegyed force-pushed the adam/per-hook-data branch from 68a5847 to b579102 Compare July 9, 2024 20:46

Base automatically changed from adam/per-hook-data to v0.8-develop July 9, 2024 20:51

Add Allowlist sample plugin, refactor test base

5c10b0d

adamegyed force-pushed the adam/sample-allowlist-hook branch from 78ae1eb to 5c10b0d Compare July 10, 2024 14:45

adamegyed merged commit d8c726c into v0.8-develop Jul 10, 2024
3 checks passed

adamegyed deleted the adam/sample-allowlist-hook branch July 10, 2024 14:49

adamegyed added a commit that referenced this pull request Jul 26, 2024

feat: [v0.8-develop] Add allowlist sample hook, refactor test base (#70)

8270523

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [v0.8-develop] Add allowlist sample hook, refactor test base #70

feat: [v0.8-develop] Add allowlist sample hook, refactor test base #70

adamegyed commented Jun 11, 2024

Zer0dot Jun 12, 2024

adamegyed Jun 12, 2024

fangting-alchemy Jun 18, 2024 •

edited

Loading

Zer0dot Jun 12, 2024

adamegyed Jun 12, 2024

fangting-alchemy Jun 18, 2024

adamegyed Jun 19, 2024

fangting-alchemy left a comment

howydev Jun 21, 2024

adamegyed Jun 24, 2024

howydev Jun 24, 2024

huaweigu left a comment

	function _getExpectedUserOpError(Call[] memory calls) internal view returns (bytes memory) {
	for (uint256 i = 0; i < calls.length; i++) {
	Call memory call = calls[i];

	(bool allowed, bool hasSelectorAllowlist) =
	allowlistPlugin.targetAllowlist(call.target, address(account1));
	if (allowed) {
	if (
	hasSelectorAllowlist
	&& !allowlistPlugin.selectorAllowlist(call.target, bytes4(call.data), address(account1))
	) {
	return abi.encodeWithSelector(
	IEntryPoint.FailedOpWithRevert.selector,
	0,
	"AA23 reverted",
	abi.encodeWithSelector(AllowlistPlugin.SelectorNotAllowed.selector)
	);
	}
	} else {
	return abi.encodeWithSelector(
	IEntryPoint.FailedOpWithRevert.selector,
	0,
	"AA23 reverted",
	abi.encodeWithSelector(AllowlistPlugin.TargetNotAllowed.selector)
	);
	}
	}

	return "";
	}

	function _getExpectedRuntimeError(Call[] memory calls) internal view returns (bytes memory) {
	for (uint256 i = 0; i < calls.length; i++) {
	Call memory call = calls[i];

	(bool allowed, bool hasSelectorAllowlist) =
	allowlistPlugin.targetAllowlist(call.target, address(account1));
	if (allowed) {
	if (
	hasSelectorAllowlist
	&& !allowlistPlugin.selectorAllowlist(call.target, bytes4(call.data), address(account1))
	) {
	return abi.encodeWithSelector(
	UpgradeableModularAccount.PreRuntimeValidationHookFailed.selector,
	address(allowlistPlugin),
	uint8(AllowlistPlugin.FunctionId.PRE_VALIDATION_HOOK),
	abi.encodeWithSelector(AllowlistPlugin.SelectorNotAllowed.selector)
	);
	}
	} else {
	return abi.encodeWithSelector(
	UpgradeableModularAccount.PreRuntimeValidationHookFailed.selector,
	address(allowlistPlugin),
	uint8(AllowlistPlugin.FunctionId.PRE_VALIDATION_HOOK),
	abi.encodeWithSelector(AllowlistPlugin.TargetNotAllowed.selector)
	);
	}
	}

feat: [v0.8-develop] Add allowlist sample hook, refactor test base #70

feat: [v0.8-develop] Add allowlist sample hook, refactor test base #70

Conversation

adamegyed commented Jun 11, 2024

Motivation

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fangting-alchemy Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fangting-alchemy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huaweigu left a comment

Choose a reason for hiding this comment

fangting-alchemy Jun 18, 2024 •

edited

Loading